Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-48644: Create a PrimaryNavigation component #179

Merged
merged 16 commits into from
Feb 19, 2025
Merged

Conversation

jonathansick
Copy link
Member

@jonathansick jonathansick commented Jan 31, 2025

This PrimaryNavigation component in Squared wraps Radix UI's NavigationMenu and provide's a replacement for the current header navigation in Squareone.

Using this new PrimaryNavigation component, this PR also adds an Apps menu item to the header navigation in Squareone that's configurable per environment.

Copy link

changeset-bot bot commented Jan 31, 2025

🦋 Changeset detected

Latest commit: 88a9654

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
squareone Minor
@lsst-sqre/squared Minor
@lsst-sqre/tsconfig Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jonathansick jonathansick force-pushed the tickets/DM-48644 branch 2 times, most recently from 94f912e to 7856e72 Compare February 5, 2025 18:42
jonathansick added a commit to lsst-sqre/phalanx that referenced this pull request Feb 7, 2025
This component wraps Radix UI's NavigationMenu and provide's a
replacement for the current header navigation in Squareone.

With NavigationMenu we'll be able to give individual menu items
submenus, and not necessarily have to treat the user menu as a special
case.

This work styles the basic PrimaryNavigation component and demonstrates
it with a storybook story.

More work is need to make the submenu appear directly below its trigger.
This is a better and more consistent user experience than a menu item
opening on hover.
This makes types available for DOM APIs, which we do use in react
libraries like Squared.
The Radix navigation menu currently has a flaw where the content
viewport is always centered under the navigation menu as a whole, rather
than under the trigger specifically. This isn't suitable for Squareone
because our navigation menu spans the entire window so the menu will
appear disassociated with the trigger.

This solution is from the GitHub issue thread and uses a
MutationObserver to update the position of the viewport.

radix-ui/primitives#1462 (comment)

It has the side-effect right now of animating the menu sliding from the
centre to the trigger on first view. Perhaps we can tweak the animations
so this doesn't happen? Also, we might need to add code to
updatePosition that takes into account the browser viewport to ensure we
don't push menu content outside the view.
This makes these utility functions more accessible as we redesign the
Gafaelfawr user menu to use the PrimaryNav component.
Ensures that the menu appears above content later in the page.
Now we test whether the menu trigger is near the right side of the menu.
If it is, we switch to using the "right" for absolute alignment. If it
is not, we use the "left". In both cases, we use the absolute alignment
rather than doing transformX to avoid having the menu slide sideways,
which doesn't make sense since the menu now opens on click rather than
hover.
This prevents the footer from shrinking by flexbox. We didn't see any
issnegative issues with this property being unset, though.
First steps to adopting the PrimaryNav into the Header. There's a weird
bug where the Login component is being rendered server-side, and hence
the Logged-in state isn't always available.

This replaces the existing HeaderNav, which composed basic Next links in
a flex nav along with a GafaelfawrUserMenu with the PrimaryNav where the
User menu is now a regular menu item.
- enableAppsMenu is a feature flag
- appLinks is an array of menu items

Add appLinks configurations

This configuration pairs with the enableAppsMenu and sets the content of
the Apps menu.
This is an initial implementation that demonstrates an Apps menu
that's displayed when the enableAppsMenu setting is added, and
shows the contents of the appLinks configuration.

This Apps menu implementation is challenging because it doesn't seem to
work well with the Next Link component, even if I use the asChild
composition method. I've tried to work around this by using a link
component that uses a span with a router.push onClick handler, but then
this breaks keyboard navigation. More work is needed.
Use the basic PrimaryNavigation.Link for external links, which becomes
an <a> tag. For internal links, use the special span with an onClick
handler to act like the Nextjs Link component, but working around the
issue of having onClick handlers on that Link component from Radix
Navigation Menu.
This sets the right cursor style for menu triggers. Previously it was
just the default arrow.
We have to set this manually since we're implementing Next links with an
onClick handler to push the router to workaround a Next Link +
NavigationMenu.Link conflict.
jonathansick added a commit to lsst-sqre/phalanx that referenced this pull request Feb 12, 2025
@jonathansick jonathansick marked this pull request as ready for review February 12, 2025 19:22
@jonathansick jonathansick merged commit 1b8bd77 into main Feb 19, 2025
10 checks passed
@jonathansick jonathansick deleted the tickets/DM-48644 branch February 19, 2025 22:33
@squareone-ci squareone-ci bot mentioned this pull request Feb 19, 2025
jonathansick added a commit to lsst-sqre/phalanx that referenced this pull request Feb 19, 2025
This version provides the new configurable Apps menu.

lsst-sqre/squareone#179
aibsen pushed a commit to lsst-sqre/phalanx that referenced this pull request Feb 24, 2025
This version provides the new configurable Apps menu.

lsst-sqre/squareone#179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant